Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format after building #1166

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Sep 17, 2023

WHAT

🤖 Generated by Copilot at 1723ad8

This pull request adds and modifies some files to integrate the Fantomas tool for formatting F# code. It enables automatic formatting on save in VS Code, and adds targets to restore and run Fantomas as part of the build process. It also adds a .fantomasignore file to exclude non-source files from formatting.

🤖 Generated by Copilot at 1723ad8

Fantomas formats
code with ease and beauty
autumn leaves no mess

📝🛠️🏗️

WHY

Let's make it harder to fail CI formatting.

HOW

🤖 Generated by Copilot at 1723ad8

  • Add a .fantomasignore file to exclude non-source files from formatting (link)
  • Enable automatic formatting on save for F# files in VS Code (link)
  • Define properties and targets for dotnet tools and fantomas in Directory.Build.targets (link)
  • Simplify format inputs to use solution directory and .fantomasignore file in Directory.Solution.targets (link)

</PropertyGroup>
</Target>
<PropertyGroup>
<_BuildProjBaseIntermediateOutputPath>$(MSBuildThisFileDirectory)build/obj/</_BuildProjBaseIntermediateOutputPath>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some Chetfu here to make this not hardcoded to build's immediate output path but i'm not sure exactly how to ask for a specific project's value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gonna be gross. I think you'll have to have a 'GetIntermediateOutputPathtarget in here that would return the value of theBaseIntermediateOutputPath` property for the project it was part of. Then you'd call that target on the build project during execution.

This dependency is why you often see features like this come in two targets:

  • one to set up any data dependencies and generate the properties/paths for any inputs/outputs to the real target
  • and the the real target the does the thing and uses the properties/items set up in the first target

this split is so that you always can set up your Inputs/Outputs and not run into issues with incrementality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald - we in the SDK get requests somewhat often for something like this: 'restore tools as part of my build'. Does anything here jump out as especially bad? Could we potentially extract some kind of recommendation in lieu of first class SDK level support?

Comment on lines +19 to +23
<Target Name="ToolRestore" BeforeTargets="Restore;CollectPackageReferences;PaketRestore" Inputs="$(_DotnetToolManifestFile)" Outputs="$(_DotnetToolRestoreOutputFile)">
<Exec Command="dotnet tool restore" WorkingDirectory="$(MSBuildThisFileDirectory)" StandardOutputImportance="High" StandardErrorImportance="High" />
<MakeDir Directories="$(_BuildProjBaseIntermediateOutputPath)"/>
<Touch Files="$(_DotnetToolRestoreOutputFile)" AlwaysCreate="True" ForceTouch="True" />
</Target>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inspired by FSharp.SystemTextJson but I worked out some more issues in MiniScaffold

Comment on lines +26 to +29
<Target Name="Format" BeforeTargets="BeforeBuild" Inputs="@(Compile)" Outputs="$(_DotnetFantomasOutputFile)" >
<Exec Command="dotnet fantomas $(MSBuildProjectDirectory)" StandardOutputImportance="High" StandardErrorImportance="High" WorkingDirectory="$(MSBuildThisFileDirectory)" />
<Touch Files="$(_DotnetFantomasOutputFile)" AlwaysCreate="True" ForceTouch="True" />
</Target>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does mean it runs per project, but it's incremental per project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the sane way to do it, since everything in MSBuild is project-based. Note that we would lose the ability to format any non-project-included files (but that's probably ok).

Comment on lines -2 to -4
<ItemGroup>
<FormatInputs Include="src/**/*.fs;src/**/*.fsi" Exclude="src/**/obj/**/*.fs" />
</ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using .fantomasignore

Comment on lines +1 to +5
test/
build/
benchmarks/
utils/
demo.fsx
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replicated from Directory.Solution.targets. We should probably start considering formatting these at some point.

Comment on lines +23 to +24
"[fsharp]": {
"editor.formatOnSave": true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not on save as well.

@TheAngryByrd
Copy link
Member Author

Oh of course tests are gonna fail because I'm using .fantomasignore now 😬

@TheAngryByrd
Copy link
Member Author

Just gonna close this for now. We'll just let CI make someone angry enough again to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants